Fix existing E2E test failures and Improve resource cleanup #37

hlts2 · 2025-04-07T12:20:23Z

WHAT

This PR includes the following changes:

Updated the version of civogo
Fixed errors encountered during test environment setup
Improved resource cleanup processing

WHY

There are plans to expand E2E testing in the future. By ensuring that the existing tests are in a working state beforehand, other members will be able to add tests more easily. If the existing tests are failing, it would delay the process, as work would need to start with fixing the existing issues. Therefore, the goal of this PR is to make sure that the existing tests are functional and ready to be expanded at any time.

I encountered the following errors when running the tests, so I've addressed them in this PR:

Cluster creation error

Unable to provision new cluster: Error: Unknown error response - status: 400 Bad Request, code: 400, reason: {"code":"firewall_missing","reason":"Please provide either instance_firewall or firewall_rule"}
Unable to provision new cluster: Error: Unknown error response - status: 400 Bad Request, code: 400, reason: {"code":"firewall_missing","reason":"Please provide either instance_firewall or firewall_rule"}

Service creation error

"Event occurred" object="default/echo-pods" fieldPath="" kind="Service" apiVersion="v1" type="Warning" reason="SyncLoadBalancerFailed" message=<
        Error syncing load balancer: failed to ensure load balancer: Error: Unknown error response - status: 400 Bad Request, code: 400, reason: {"code":"firewall_missing","reason":"Please provide either instance_firewall or firewall_rule"}

Additionally, resources were not being released correctly, preventing tests from running consecutively. This PR includes fixes related to proper resource cleanup, particularly using Context and handling cleanup processes.

Test

Create Reserved IP Addresses in the UI dashboard.
- I believe it can be executed without pre-creation, but since I had something pre-created reserved ip in my environment, I used that. (name: ccm-e2e-test-ip)
Configure .env for test execution
Execute the following command

go test -count=1 -timeout 20m -v ./e2e/...

Test Result

All E2E tests have passed.

Creating mirror deployment
Create a reserved IP for e2e test (if it doesn't exist)
Creating Service
I0407 22:23:32.442038 3872584 event.go:307] "Event occurred" object="default/echo-pods" fieldPath="" kind="Service" apiVersion="v1" type="Normal" reason="EnsuringLoadBalancer" message="Ensuring load balancer"
Updating service with reserved IP annotation
E0407 22:23:41.290795 3872584 loadbalancer.go:256] Loadbalancer is not available, state: pending_ip_allocation
......
PASS
Attempting Test Cleanup
Deleting Cluster: 392b4b11-e801-4451-bab8-f59d36afbcf3
ok      github.com/civo/civo-cloud-controller-manager/e2e       812.994s

FYI

⚠️ This PR does not include any logic changes related to the component. Additionally, since it will not be released immediately, there will be no impact on users.

Signed-off-by: hlts2 <[email protected]>

This reverts commit a69ce30.

Signed-off-by: hlts2 <[email protected]>

hlts2 · 2025-04-07T12:44:26Z

go.mod

@@ -3,7 +3,7 @@ module github.com/civo/civo-cloud-controller-manager
 go 1.24.1

 require (
-	github.com/civo/civogo v0.3.61
+	github.com/civo/civogo v0.3.96


Note
Since civo-cloud-controller-manager will not be released immediately, there should be no impact on the existing environment. However, if the version update is unnecessary, I will revert this changes. 🙏

hlts2 · 2025-04-07T12:45:28Z

e2e/Readme.md

@@ -5,7 +5,7 @@ e2e tests for the Civo Cloud Controller Manager.
 These tests can be run:

 ```bash
-CIVO_API_KEY=.... go test --timeout 30 -v ./e2e/...
+CIVO_API_KEY=.... go test -timeout 20m -v ./...


Note
The command was incorrect, so I have fixed it. Additionally, since it didn't finish within 30 seconds, I have changed the timeout to 20 minutes.

hlts2 · 2025-04-07T12:46:41Z

e2e/loadbalancer_test.go

 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/intstr"
 )

 func TestLoadbalancerBasic(t *testing.T) {
-
+	ctx := t.Context()


Note
Since t.Context was introduced in Go 1.24, I used it. This will allow for proper cancellation handling.

hlts2 · 2025-04-07T12:48:31Z

e2e/loadbalancer_test.go

 	g.Expect(err).ShouldNot(HaveOccurred())
-	defer e2eTest.tenantClient.Delete(context.TODO(), mirrorDeploy)
+	t.Cleanup(func() {
+		ctx, cancel := context.WithTimeout(context.Background(), time.Minute)


Note
It is recommended to use t.Cleanup in tests, so I used it. Additionally, since t.Context cannot be used internally, I am using a context with a timeout.

hlts2 · 2025-04-07T12:50:25Z

e2e/suite_test.go

 			},
 		},
+		FirewallRule: "443;80",


Note
I have set the firewall_rule based on the following error that occurred during cluster creation.

Unable to provision new cluster: Error: Unknown error response - status: 400 Bad Request, code: 400, reason: {"code":"firewall_missing","reason":"Please provide either instance_firewall or firewall_rule"} Unable to provision new cluster: Error: Unknown error response - status: 400 Bad Request, code: 400, reason: {"code":"firewall_missing","reason":"Please provide either instance_firewall or firewall_rule"}

hlts2 · 2025-04-07T12:52:45Z

e2e/loadbalancer_test.go


 	lbls := map[string]string{"app": "mirror-pod"}
 	// Create a service of type: LoadBalancer
 	svc := &corev1.Service{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      "echo-pods",
 			Namespace: "default",
+			Annotations: map[string]string{
+				"kubernetes.civo.com/firewall-id": e2eTest.cluster.FirewallID,


Note
After correcting the previous comment, the following error occurred during service creation, so I made adjustments based on the details of that error.

"Event occurred" object="default/echo-pods" fieldPath="" kind="Service" apiVersion="v1" type="Warning" reason="SyncLoadBalancerFailed" message=< Error syncing load balancer: failed to ensure load balancer: Error: Unknown error response - status: 400 Bad Request, code: 400, reason: {"code":"firewall_missing","reason":"Please provide either instance_firewall or firewall_rule"}

hlts2 · 2025-04-07T12:54:13Z

e2e/loadbalancer_test.go

@@ -79,6 +91,7 @@ func TestLoadbalancerProxy(t *testing.T) {
 			Namespace: "default",
 			Annotations: map[string]string{
 				"kubernetes.civo.com/loadbalancer-enable-proxy-protocol": "send-proxy",
+				"kubernetes.civo.com/firewall-id":                        e2eTest.cluster.FirewallID,


Note
Same as the below comment.
https://github.com/civo/civo-cloud-controller-manager/pull/37/files#r2031178002

hlts2 · 2025-04-07T12:54:30Z

e2e/loadbalancer_test.go

+				Name:      "echo-pods",
+				Namespace: "default",
+				Annotations: map[string]string{
+					"kubernetes.civo.com/firewall-id": e2eTest.cluster.FirewallID,


Note
Same as the below comment.
https://github.com/civo/civo-cloud-controller-manager/pull/37/files#r2031178002

hlts2 · 2025-04-07T12:56:24Z

e2e/suite_test.go

@@ -142,7 +142,7 @@ func (e *E2ETest) provisionCluster() {
 		CivoRegion = "LON1"
 	}

-	CivoURL := os.Getenv("CIVO_URL")
+	CivoURL := os.Getenv("CIVO_API_URL")


Note
I changed it because the name was different from the expected one.

hlts2 · 2025-04-07T12:57:07Z

e2e/suite_test.go

@@ -178,9 +178,10 @@ func (e *E2ETest) provisionCluster() {
 		Pools: []civogo.KubernetesClusterPoolConfig{
 			{
 				Count: 2,
-				Size:  "g4s.kube.xsmall",
+				Size:  "g4s.kube.small",


Note
Just to be safe, I changed it to "small" in case it doesn't start.

hlts2 · 2025-04-07T13:08:06Z

e2e/loadbalancer_test.go

 	}, "2m", "5s").ShouldNot(BeNil())
 }

-func TestLoadbalancerHTTPForwardFor(t *testing.T) {


Note
The following test was added three years ago, but after catching up on an internal Slack message(Apr 5th, 2024) about not returning x-forwarded-for, I decided to remove it this time.
#8

@DMajrekar
Is this understanding correct? 🤔 If so, I will make the necessary changes to the README in a separate PR.

agreed that the current test is out of date and can be removed

We now support this via a proxy_protocol flag

https://www.civo.com/docs/kubernetes/load-balancers#proxy-protocol

we should either add a test here, or add and link an issue for a future addition of a test for this functionally. Your choice

opened #38 it in case it doesn't make it in

Thank you for your comment 🙏
I'll go ahead and add a test for this functionality in the next PR to ensure it's covered.
issue: #38

@johndietz Thank you for creating issue! 🙏

johndietz

pr, description, and comments are all great @hlts2 thank you so much

hlts2 · 2025-04-09T06:54:27Z

@DMajrekar @johndietz
Thank you for your review 🙇 I will merge this PR 🚀

hlts2 added 11 commits April 2, 2025 11:56

fix: upgrade civogo and bugfix useragent settings

ad8a05d

Signed-off-by: hlts2 <[email protected]>

fix: bugfix and todo comment

a238dbe

Signed-off-by: hlts2 <[email protected]>

fix: add region to ip request

7198c0d

Signed-off-by: hlts2 <[email protected]>

fix: deleted unnecessary code

2c1246a

Signed-off-by: hlts2 <[email protected]>

fix: add resource cleanup logic

693b35d

Signed-off-by: hlts2 <[email protected]>

fix: use cleanup function

01fd65c

Signed-off-by: hlts2 <[email protected]>

fix: revert variable changes

9acf100

Signed-off-by: hlts2 <[email protected]>

fix: deleted unnecessary test

55d0d20

Signed-off-by: hlts2 <[email protected]>

fix: add test error handling

a69ce30

Signed-off-by: hlts2 <[email protected]>

Revert "fix: add test error handling"

f7a2e61

This reverts commit a69ce30.

fix: add timeout context

e0b31c5

Signed-off-by: hlts2 <[email protected]>

hlts2 commented Apr 7, 2025

View reviewed changes

hlts2 self-assigned this Apr 7, 2025

hlts2 commented Apr 7, 2025

View reviewed changes

hlts2 changed the title ~~[WIP]: Fix Existing Test Failures and Improve Resource Cleanup for E2E Testing Expansion~~ [WIP]: Fix existing E2E test failures and Improve resource cleanup for E2E testing expansion Apr 7, 2025

hlts2 changed the title ~~[WIP]: Fix existing E2E test failures and Improve resource cleanup for E2E testing expansion~~ [WIP]: Fix existing E2E test failures and Improve resource cleanup Apr 7, 2025

hlts2 marked this pull request as ready for review April 7, 2025 13:40

hlts2 requested review from DMajrekar, johndietz, jarededwards and jokestax April 7, 2025 13:41

hlts2 changed the title ~~[WIP]: Fix existing E2E test failures and Improve resource cleanup~~ Fix existing E2E test failures and Improve resource cleanup Apr 7, 2025

johndietz approved these changes Apr 9, 2025

View reviewed changes

johndietz mentioned this pull request Apr 9, 2025

proxy-protocol flag needs a new test #38

Closed

hlts2 merged commit 5570c6b into master Apr 9, 2025
4 checks passed

hlts2 deleted the fix/ccm-version branch April 9, 2025 06:54

hlts2 mentioned this pull request Apr 14, 2025

Add E2E test for proxy protocol #39

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix existing E2E test failures and Improve resource cleanup #37

Fix existing E2E test failures and Improve resource cleanup #37

hlts2 commented Apr 7, 2025 •

edited

Loading

hlts2 Apr 7, 2025 •

edited

Loading

hlts2 Apr 7, 2025

hlts2 Apr 7, 2025 •

edited

Loading

hlts2 Apr 7, 2025 •

edited

Loading

hlts2 Apr 7, 2025

hlts2 Apr 7, 2025

hlts2 Apr 7, 2025

hlts2 Apr 7, 2025

hlts2 Apr 7, 2025

hlts2 Apr 7, 2025

hlts2 Apr 7, 2025 •

edited

Loading

hlts2 Apr 7, 2025 •

edited

Loading

DMajrekar Apr 8, 2025

johndietz Apr 9, 2025

hlts2 Apr 9, 2025 •

edited

Loading

hlts2 Apr 9, 2025

johndietz left a comment

hlts2 commented Apr 9, 2025

Fix existing E2E test failures and Improve resource cleanup #37

Fix existing E2E test failures and Improve resource cleanup #37

Conversation

hlts2 commented Apr 7, 2025 • edited Loading

WHAT

WHY

Test

Test Result

FYI

hlts2 Apr 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hlts2 Apr 7, 2025 • edited Loading

Choose a reason for hiding this comment

hlts2 Apr 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hlts2 Apr 7, 2025 • edited Loading

Choose a reason for hiding this comment

hlts2 Apr 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hlts2 Apr 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johndietz left a comment

Choose a reason for hiding this comment

hlts2 commented Apr 9, 2025

hlts2 commented Apr 7, 2025 •

edited

Loading

hlts2 Apr 7, 2025 •

edited

Loading

hlts2 Apr 7, 2025 •

edited

Loading

hlts2 Apr 7, 2025 •

edited

Loading

hlts2 Apr 7, 2025 •

edited

Loading

hlts2 Apr 7, 2025 •

edited

Loading

hlts2 Apr 9, 2025 •

edited

Loading